Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

openssl: add 3.0.8 + add GitHub as mirror #15802

Merged
merged 2 commits into from
Feb 18, 2023

Conversation

Croydon
Copy link
Contributor

@Croydon Croydon commented Feb 7, 2023

https://www.openssl.org/news/openssl-3.0-notes.html

For 1.1.1t see #15860


AbrilRBS
AbrilRBS previously approved these changes Feb 7, 2023
@ghost
Copy link

ghost commented Feb 7, 2023

I detected other pull requests that are modifying openssl/3.x.x recipe:

This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there.

prince-chrismc
prince-chrismc previously approved these changes Feb 7, 2023
@conan-center-bot

This comment has been minimized.

@Croydon Croydon closed this Feb 7, 2023
@Croydon Croydon reopened this Feb 7, 2023
@gegles
Copy link
Contributor

gegles commented Feb 8, 2023

What's the status on this? Is anybody actively looking into it?

Much appreciated!

@prince-chrismc prince-chrismc self-assigned this Feb 8, 2023
@prince-chrismc
Copy link
Contributor

What's the status on this? Is anybody actively looking into it?

You can checkout this bot prince-chrismc/conan-center-index-pending-review#1 which will give you a rundown of all the PRs and the bottom has graphs about how long they are taking

@prince-chrismc
Copy link
Contributor

Ill need to check why this failed again from my MacBook 😕

@Croydon
Copy link
Contributor Author

Croydon commented Feb 8, 2023

Ill need to check why this failed again from my MacBook 😕

Is this something that happened recently? Can you give me a cross-reference please?

This PR doesn't modify 3.0.5 at all, so it is weird.

@gegles
Copy link
Contributor

gegles commented Feb 8, 2023

Any movement on this? We're waiting on this to release our product so we can incorporate the latest OpenSSL security fixes...

@conan-center-bot

This comment has been minimized.

@Croydon
Copy link
Contributor Author

Croydon commented Feb 9, 2023

Random thing that came up with a search: Maybe the openssl config files is making problems while loading the legacy stuff nodejs/node#43184

Env var OPENSSL_CONF on a macOS env and the content of the file might be worth checking out.

The question remains what changed in the CI environment that surfaced this error? It is really bad that critical updates like these are blocked by unrelated bugs.

Maybe we should disable the tests with the legacy providers to get this merged first.

@conan-center-bot

This comment has been minimized.

@clauverjat
Copy link

clauverjat commented Feb 9, 2023

Random thing that came up with a search: Maybe the openssl config files is making problems while loading the legacy stuff nodejs/node#43184

The question remains what changed in the CI environment that surfaced this error? It is really bad that critical updates like these are blocked by unrelated bugs.

Maybe we should disable the tests with the legacy providers to get this merged first.

I am following this issue closely as we are waiting on this PR being merged to make a new release of our software to respond to the disclosed openssl vulnerabilities. I could not agree more with your statement about how damaging it is from a security standpoint.

Hope this issue will be resolved soon.

Thanks for your work !

@Croydon
Copy link
Contributor Author

Croydon commented Feb 9, 2023

We are actually already skipping tests for OpenSSL 1.x on macOS armv8 shared

@SpaceIm
Copy link
Contributor

SpaceIm commented Feb 9, 2023

We are actually already skipping tests for OpenSSL 1.x on macOS armv8 shared

yes but for another reason (and this skip_test in 1.x.x branch should be removed because CMake has been upgraded on Macos agents).

@conan-center-bot

This comment has been minimized.

@prince-chrismc
Copy link
Contributor

Do that plus add some links for reviewers and it can go in :)

@SpaceIm
Copy link
Contributor

SpaceIm commented Feb 12, 2023

It's a suggestion which would make sense given the error and OpenSSL documentation regarding env var and loading mechanism of modules. I let OpenSSL experts here give their opinion and provide the fix.

@daniel-heater-imprivata

Can this be replicated locally?

I tried it on my Mac with

[settings]
arch=x86_64
build_type=Debug
compiler=apple-clang
compiler.libcxx=libc++
compiler.version=14
os=Macos
[options]
openssl:shared=False

Both conan create . openssl/3.0.8@ --build=missing and conan test test_package openssl/3.0.8@ succeed.

@gegles
Copy link
Contributor

gegles commented Feb 13, 2023

@prince-chrismc @SpaceIm anything that can be done to get this merged faster? We're holding off an entire release because of this.

Much appreciated!

@prince-chrismc
Copy link
Contributor

@gegles please contribute the fix 🙏 that will ve the fastest way :)

@daniel-heater-imprivata

I have not been able to reproduce locally and I do not have access to the full CI logs. What is the best way to contribute?

Also, I diffed the 3.0.7 and 3.0.8 release tags and this changes looks like it might be relevant:

diff --git a/crypto/md5/build.info b/crypto/md5/build.info
index a1e28c8153..9a32538606 100644
--- a/crypto/md5/build.info
+++ b/crypto/md5/build.info
@@ -23,7 +23,7 @@ SOURCE[../../libcrypto]=$COMMON
 # default provider.  A no-deprecated build removes the external definition from
 # libcrypto and this means that the code needs to be in liblegacy.  However,
 # when building without 'dso', liblegacy is included in libcrypto.
-IF[{- !$disabled{dso} -}]
+IF[{- !$disabled{module} && !$disabled{shared} -}]
   SOURCE[../../providers/liblegacy.a]=$COMMON
 ENDIF

@prince-chrismc
Copy link
Contributor

prince-chrismc commented Feb 13, 2023

You can access the logs from
the comments of the bot ... the last one is https://c3i.jfrog.io/c3i/misc/summary.html?json=https://c3i.jfrog.io/c3i/misc/logs/pr/15802/10-configs/macos-clang/openssl/3.0.8//summary.json

EDIT: I suggested docker images but that wont work for macos nodes

@daniel-heater-imprivata

EDIT: I suggested docker images but that wont work for macos nodes

osxcross?

@Croydon
Copy link
Contributor Author

Croydon commented Feb 14, 2023

I can't reproduce this locally either.
It isn't related to OpenSSL changes as it happens with e.g. 3.0.7 as well.

I'm not sure about those environment variables, but why would they be suddenly required? And why only Windows and macOS?

@jcar87 jcar87 marked this pull request as draft February 15, 2023 16:54
@jcar87
Copy link
Contributor

jcar87 commented Feb 15, 2023

Hi @Croydon - we are currently investigating this.

Could you please add the following to the recipe so that we can verify the build configuration? I've changed the PR to draft while we continue to investigate this

diff --git a/recipes/openssl/3.x.x/conanfile.py b/recipes/openssl/3.x.x/conanfile.py
index 05de257a5..2a7b19e88 100644
--- a/recipes/openssl/3.x.x/conanfile.py
+++ b/recipes/openssl/3.x.x/conanfile.py
@@ -610,6 +610,7 @@ class OpenSSLConan(ConanFile):
                 self._create_targets()
                 with self._make_context():
                     self._make()
+                    self.run("perl source_subfolder/configdata.pm --dump")
 
     @property
     def _win_bash(self)

☝️ Please disregard this, see below.

@jcar87
Copy link
Contributor

jcar87 commented Feb 15, 2023

There are two "loadable" modules in the package, irrespective of whether the recipe is built as shared or not:

├── lib
│   ├── cmake
│   │   └── conan-official-openssl-variables.cmake
│   ├── libcrypto.a
│   ├── libssl.a
│   └── ossl-modules
│       ├── fips.dylib
│       └── legacy.dylib
└── licenses

The issue is that OpenSSL will look in the directory provided at install time - this is why we cannot reproduce this locally, because those directories will exist on everyone's local machine as part of Conan create.

An easier way to test this is to clear the binaries from the Conan cache, and run:

conan test . openssl/3.0.7@ 

and let Conan fetch the OpenSSL binaries from Conan Center - we should now see the same error. Inspecting the process, I can see it tries to look in a path inside /Users/jenkins, which doesn't exist on my machine, and this is likely what's heppening in those failed builds above.

Adding this to the package_info seems to fix it for me, as @SpaceIm mentions above.

diff --git a/recipes/openssl/3.x.x/conanfile.py b/recipes/openssl/3.x.x/conanfile.py
index 05de257a5..34311f48a 100644
--- a/recipes/openssl/3.x.x/conanfile.py
+++ b/recipes/openssl/3.x.x/conanfile.py
@@ -756,3 +756,9 @@ class OpenSSLConan(ConanFile):
         self.cpp_info.components["crypto"].names["cmake_find_package_multi"] = "Crypto"
         self.cpp_info.components["ssl"].names["cmake_find_package"] = "SSL"
         self.cpp_info.components["ssl"].names["cmake_find_package_multi"] = "SSL"
+
+        openssl_modules_dir = os.path.join(self.package_folder, "lib", "ossl-modules")
+        self.runenv_info.define_path("OPENSSL_MODULES", openssl_modules_dir)
+
+        # For legacy 1.x downstream consumers, remove once recipe is 2.0 only:
+        self.env_info.OPENSSL_MODULES = openssl_modules_dir

This is a rough patch that should fix the issue - I'd advise to also add some comments in the recipe clarifying the need for this. THis env-var is documented in: https://www.openssl.org/docs/man3.0/man7/openssl-env.html

Thanks!

I'm not sure about those environment variables, but why would they be suddenly required? And why only Windows and macOS?

@Croydon - simple coincidence. The paths are absolute to the package folder inside the Conan cache, and may have just coincided when conan test was run in the past. It may still be coinciding on Linux out of luck, unless OpenSSL has different logic to load these modules on Linux.

Signed-off-by: Uilian Ries <uilianries@gmail.com>
@uilianries uilianries dismissed stale reviews from prince-chrismc and exief via b8f80e8 February 16, 2023 12:58
@uilianries
Copy link
Member

@Croydon I just pushed @jcar87 suggestions to this branch, please, don't forget to rebase on your local copy.

@@ -610,6 +610,7 @@ def build(self):
self._create_targets()
with self._make_context():
self._make()
self.run("perl source_subfolder/configdata.pm --dump")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it's code to dump OpenSSL configuration

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was debug code from us testing it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the results are posted i will remove it :)

Suggested change
self.run("perl source_subfolder/configdata.pm --dump")

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it might be useful for troubleshooting in future, e.g. debugging bugs, it's worth keeping

self.runenv_info.define_path("OPENSSL_MODULES", openssl_modules_dir)

# For legacy 1.x downstream consumers, remove once recipe is 2.0 only:
self.env_info.OPENSSL_MODULES = openssl_modules_dir
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we set more variables listed here? https://www.openssl.org/docs/man3.0/man7/openssl-env.html

@Croydon
Copy link
Contributor Author

Croydon commented Feb 17, 2023

Thanks everyone for investigating this.

The CI's main job failed without a reported error. I'm going to restart the CI

@Croydon Croydon closed this Feb 17, 2023
@Croydon Croydon reopened this Feb 17, 2023
@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ✔️

All green in build 12 (b8f80e84e1e702ec679b2ca5b01c4a8a5b08aee2):

  • openssl/3.0.7@:
    All packages built successfully! (All logs)

  • openssl/3.0.5@:
    All packages built successfully! (All logs)

  • openssl/3.0.8@:
    All packages built successfully! (All logs)


Conan v2 pipeline (informative, not required for merge) ❌

Note: Conan v2 builds are informative and they are not required for the PR to be merged.

The v2 pipeline failed. Please, review the errors and note this will be required for pull requests to be merged in the near future.

See details:

Failure in build 11 (b8f80e84e1e702ec679b2ca5b01c4a8a5b08aee2):

  • openssl/3.0.7@:
    Error running command conan export --name openssl --version 3.0.7 recipes/openssl/3.x.x/conanfile.py:

    ERROR: Error loading conanfile at '/home/conan/w/prod-v2_cci_PR-15802/recipes/openssl/3.x.x/conanfile.py': Unable to load conanfile in /home/conan/w/prod-v2_cci_PR-15802/recipes/openssl/3.x.x/conanfile.py
      File "<frozen importlib._bootstrap_external>", line 728, in exec_module
      File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
      File "/home/conan/w/prod-v2_cci_PR-15802/recipes/openssl/3.x.x/conanfile.py", line 6, in <module>
        from conans import AutoToolsBuildEnvironment, tools
    ImportError: cannot import name 'AutoToolsBuildEnvironment' from 'conans' (/opt/pyenv/versions/3.7.13/lib/python3.7/site-packages/conans/__init__.py)
    
  • openssl/3.0.5@:
    Error running command conan export --name openssl --version 3.0.5 recipes/openssl/3.x.x/conanfile.py:

    ERROR: Error loading conanfile at '/home/conan/w/prod-v2_cci_PR-15802/recipes/openssl/3.x.x/conanfile.py': Unable to load conanfile in /home/conan/w/prod-v2_cci_PR-15802/recipes/openssl/3.x.x/conanfile.py
      File "<frozen importlib._bootstrap_external>", line 728, in exec_module
      File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
      File "/home/conan/w/prod-v2_cci_PR-15802/recipes/openssl/3.x.x/conanfile.py", line 6, in <module>
        from conans import AutoToolsBuildEnvironment, tools
    ImportError: cannot import name 'AutoToolsBuildEnvironment' from 'conans' (/opt/pyenv/versions/3.7.13/lib/python3.7/site-packages/conans/__init__.py)
    
  • openssl/3.0.8@:
    Error running command conan export --name openssl --version 3.0.8 recipes/openssl/3.x.x/conanfile.py:

    ERROR: Error loading conanfile at '/home/conan/w/prod-v2_cci_PR-15802/recipes/openssl/3.x.x/conanfile.py': Unable to load conanfile in /home/conan/w/prod-v2_cci_PR-15802/recipes/openssl/3.x.x/conanfile.py
      File "<frozen importlib._bootstrap_external>", line 728, in exec_module
      File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
      File "/home/conan/w/prod-v2_cci_PR-15802/recipes/openssl/3.x.x/conanfile.py", line 6, in <module>
        from conans import AutoToolsBuildEnvironment, tools
    ImportError: cannot import name 'AutoToolsBuildEnvironment' from 'conans' (/opt/pyenv/versions/3.7.13/lib/python3.7/site-packages/conans/__init__.py)
    

Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability.

@jcar87 jcar87 marked this pull request as ready for review February 18, 2023 10:49
@conan-center-bot conan-center-bot merged commit abff546 into conan-io:master Feb 18, 2023
@Croydon Croydon deleted the openssl-new-versions branch February 18, 2023 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.